typing: Fix signature of Redis.transaction#4002
Conversation
The return type depends on the value of the value_from_callable kwarg. Indicate this with type hinting. Signed-off-by: Stephen Finucane <stephen@that.guru>
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
Hi @stephenfin , thank you for your contribution! We will have a look at it. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 0a429bd. Configure here.
| *watches: str | None, | ||
| shard_hint: Any = None, | ||
| value_from_callable: bool = False, | ||
| watch_delay: int | None = None, |
There was a problem hiding this comment.
watch_delay type too narrow compared to async counterpart
Medium Severity
The watch_delay parameter is typed as int | None in all three signatures (both overloads and the implementation), but the async version of this same method in redis/asyncio/client.py correctly types it as Optional[float]. Since time.sleep() accepts float values, the int annotation is too restrictive and would cause type checkers to reject valid fractional-second delays like 0.5. This needs to be float | None to match the async counterpart and the actual runtime behavior.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 0a429bd. Configure here.
There was a problem hiding this comment.
I'd actually spotted and fixed this locally (albeit I went with int | float | None) as part of a larger series but I hadn't pushed it since I wasn't sure this was going to be merged.
@petyaslavova it looks like you're doing most of the type work here. Would you like to take ownership of this and fix as necessary? Or would you rather I push the fix? Also, I have a larger patch to make many of the *Commands classes and make them subclass Generic[AnyStr] since they accept and return both str and bytes. Do you want that or are you planning to fix that yourself?
There was a problem hiding this comment.
Hey @stephenfin, I’ll take a pass at this—hopefully I can work on it next week.
Regarding the suggestion to extract a generic parent class for *Commands, I’d prefer to hold off on that for now. We’ve introduced quite a few changes in the current release, and in case stabilization fixes are needed, it would be better to pause on larger refactors for a while.
3 participants
Description of change
The return type depends on the value of the
value_from_callablekwarg. Indicate this with type hinting.Pull Request check-list
Note
Low Risk
Low risk: this is a type-hint/signature refinement for
Redis.transactionwith no functional behavior changes beyond argument plumbing.Overview
Improves typing for
Redis.transactionby introducing a genericTypeVarand@overloadsignatures so the return type is correctly inferred based onvalue_from_callable(pipeline results vs callable return value).The method signature is expanded to explicitly accept
shard_hint,watch_delay, and typedwatches, replacing the prior**kwargs-only extraction for these parameters.Reviewed by Cursor Bugbot for commit 449e1a4. Bugbot is set up for automated code reviews on this repo. Configure here.